Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

kops: Don't try to split --set flags on commas #16125

Closed
wants to merge 1 commit into from
Closed

kops: Don't try to split --set flags on commas #16125

wants to merge 1 commit into from

Conversation

justinsb
Copy link
Member

@justinsb justinsb commented Nov 23, 2023 •
edited
Loading

Passing quotes in very difficult with the --set flag, because the
pflag library tries to parse it using strict CSV semantics. We switch
to using the StringArrayVar, which does not attempt splitting.

This is a breaking change; compound flags must now be written as two flags, but I think that is clearer.

Example: --set=a,b must now be --set=a --set=b

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 23, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from justinsb. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@justinsb justinsb mentioned this pull request Nov 23, 2023
Merged
@justinsb
kops: Don't try to split --set flags on commas
b06dc2b 
Passing quotes in very difficult with the --set flag, because the
pflag library tries to parse it using strict CSV semantics.  We switch
to using the StringArrayVar, which does not attempt splitting.

This is a breaking change; compound flags must now be written as two flags, but I think that is clearer.

Example: `--set=a,b` must now be `--set=a --set=b`
upodroid added a commit to borg-land/kops that referenced this pull request Nov 23, 2023
@upodroid
test justin's patch in kubernetes#16125
8511727
upodroid added a commit to borg-land/kops that referenced this pull request Nov 23, 2023
@upodroid
test justin's patch in kubernetes#16125
99b5fdf
@hakman
Copy link
Member

hakman commented Nov 23, 2023

This would break some scenarios, like scalability tests.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 23, 2023
@justinsb
Copy link
Member Author

This would break some scenarios, like scalability tests.

Ah, bummer. I do think --set=foo=bar --set=foo2=bar2 is more readable than --set=foo=bar,foo2=bar2, particularly when the paths get long, but it is a breaking change.

We could also just do a more naive splitting on commas. We can also use the CSV splitter with LazyQuotes=true, this was proposed in spf13/pflag#371

@hakman
Copy link
Member

hakman commented Nov 24, 2023 •
edited
Loading

I think just fixing the scalability scenario would be enough. It's not such a bad change.
There may be other problems with the way we de-duplicate args in kubetest (I guess it's why I did this ugly thing).

kops/tests/e2e/scenarios/scalability/run-test.sh

Line 89 in b4d787f

create_args=("--network-cidr=10.0.0.0/16,10.1.0.0/16,10.2.0.0/16,10.3.0.0/16,10.4.0.0/16,10.5.0.0/16,10.6.0.0/16,10.7.0.0/16,10.8.0.0/16,10.9.0.0/16,10.10.0.0/16,10.11.0.0/16,10.12.0.0/16")

@justinsb justinsb mentioned this pull request Nov 24, 2023
Merged
@justinsb
Copy link
Member Author

I do think you raise a good point though, and it's possible that people are setting compound flags "in the wild" that we don't know about, so I don't particularly want to break them if we can avoid it.

The fix is actually pretty easy, in #16128 - we just need to set LazyQuotes: true. It's a bit harder than it should be, but it would be very easy if pflag merged the support.

My 2c: #16128 unblocks us without breaking anyone...

@hakman
Copy link
Member

hakman commented Nov 26, 2023

#16128 was merged.
/close

@k8s-ci-robot
Copy link
Contributor

@hakman: Closed this PR.

In response to this:

#16128 was merged.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johngmyers johngmyers Awaiting requested review from johngmyers

@olemarkus olemarkus Awaiting requested review from olemarkus

Assignees
No one assigned
Labels
area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@justinsb @k8s-ci-robot @hakman